Add tuple support in interactive prompts #1333

spalladino · 2019-12-13T20:28:13Z

Fixes #1330. Tuples can be entered as comma separated, or as comma separated enclosed in parenthesis.

frangio

Thank you @spalladino! The logic looks solid.

frangio · 2019-12-16T17:54:39Z

packages/lib/src/utils/ABIs.ts

@@ -66,9 +66,17 @@ export function getABIFunction(contract: Contract, methodName: string, args: any
  }
 }

+export function getABIType(arg: any): string {


I think InputInfo is the right type.

Suggested change

export function getABIType(arg: any): string {

export function getABIType(arg: InputInfo): string {

Also what do you think about renaming this function so that it's clear it's for display purposes? displayABIType, getABITypeDisplay, getABITypeLabel, ... Not sure. I also think it's kind of redundant for the name to say ABI, since it's the name of the module it's contained in.

Good point on the naming. Changed it to getArgTypeLabel.

frangio · 2019-12-16T18:00:50Z

packages/cli/src/utils/input.ts

@@ -238,3 +250,35 @@ export function validateSalt(salt: string, required = false) {
    throw new Error(`Invalid salt ${salt}, must be an uint256 value.`);
  }
 }
+
+export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string {


I don't think "placeholder" is very accurate. What about getSampleInput?

Like it! The original name was placeholder because the idea was to use it as a placeholder when presenting the question, but it was after that we noted that inquirer does not support placeholders (it only displays them when they are the default value).

frangio · 2019-12-16T18:02:19Z

packages/cli/src/utils/input.ts

@@ -46,18 +48,24 @@ function quoteArguments(args: string) {
  return args;
 }

-export function parseArg(input: string | string[], type: string): any {
+export function parseArg(input: string | string[], { type, components }: Pick<MethodArg, 'type' | 'components'>): any {


Why not use the entire MethodArg type here? I imagine that using Pick simplifies the tests? Is there another reason?

If we insist on not using the entire type, I think we should define a type alias in this file. It represents an important concept, and it's used twice in the file.

The problem was in the recursive call, since the types for an array (for example) are not named. I agree with giving it a proper name, just created MethodArgType for that.

frangio · 2019-12-16T18:04:54Z

packages/cli/src/utils/input.ts

-  // TODO: Handle tuples in ABI specification
+  // Tuples: recursively parse
+  if (type === 'tuple') {
+    const inputs = typeof input === 'string' ? parseArray(stripParens(input), '(', ')') : input;


I found it weird that parseArray wouldn't handle the delimiters itself. What do you think about doing that?

Also, why do we allow the input to not be surrounded by delimiters? In what cases is that used? Does that apply to tuples in the same way that it applies to arrays?

I found it weird that parseArray wouldn't handle the delimiters itself. What do you think about doing that?

Good point, I think it could be refactored. That said, given that this routine is pretty isolated (and working!), I wouldn't risk changing it now.

Also, why do we allow the input to not be surrounded by delimiters? In what cases is that used? Does that apply to tuples in the same way that it applies to arrays?

When the user is prompted to enter an array, they can just enter a comma separated list, without the brackets, for simplicity. Same applies for tuples.

frangio · 2019-12-16T18:33:01Z

packages/cli/src/prompts/method-params.ts

@@ -59,18 +59,20 @@ function getCommandProps(
    return {
      ...accum,
      [arg.name]: {
-        message: arg.name ? `${arg.name} (${arg.type}):` : `(${arg.type}):`,
+        message: argLabel(arg),


Note that the previous string had a colon at the end, and argLabel doesn't (which is okay because in the other place where it's used there was no colon).

Suggested change

message: argLabel(arg),

message: argLabel(arg) + ':',

Good catch!

frangio · 2019-12-16T18:38:45Z

packages/cli/src/utils/input.ts

@@ -46,18 +48,24 @@ function quoteArguments(args: string) {
  return args;
 }

-export function parseArg(input: string | string[], type: string): any {
+export function parseArg(input: string | string[], { type, components }: Pick<MethodArg, 'type' | 'components'>): any {


Out of curiosity, in what cases is input an array?

IIRC, parseArray does not parse just the top-level arrays, but it returns arrays of arrays (as many levels deep as needed). So, the parseArg recursive call in the case of arrays may be done with an array instead of a string.

And writing this I'm thinking that maybe arrays of tuples (or viceversa) may not work...

packages/cli/src/utils/input.ts

frangio · 2019-12-16T18:42:25Z

packages/cli/src/utils/input.ts

@@ -261,8 +274,10 @@ export function getPlaceholder(type: string): string {
    return '0x1df62f291b2e969fb0849d99d9ce41e2f137006e';
  } else if (type === 'string') {
    return 'Hello world';
+  } else if (type === 'tuple' && components) {


Are there cases when components is undefined?

frangio · 2019-12-16T18:45:41Z

packages/cli/src/utils/input.ts

@@ -238,3 +250,35 @@ export function validateSalt(salt: string, required = false) {
    throw new Error(`Invalid salt ${salt}, must be an uint256 value.`);
  }
 }
+
+export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string {


The compiler isn't catching this because we're not running it in strict mode. It will be a bit of an undertaking to enable it, but I would highly recommend we do.

Suggested change

export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string {

export function getPlaceholder(arg: Pick<MethodArg, 'type' | 'components'>): string | null {

Good one. And agree on the strict mode!

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

spalladino · 2019-12-16T20:55:10Z

@frangio updated!

frangio · 2019-12-16T21:03:25Z

packages/lib/src/utils/ABIs.ts

@@ -137,5 +146,6 @@ export function callDescription(method: any, args: string[]): string {
 export default {
  buildCallData,
  getABIFunction,
+  getABIType: getArgTypeLabel,


Can't we remove this here? It didn't exist before this PR so there are no backwards compatibility issues.

Yes, that was the auto-rename backfiring - sorry about it

spalladino · 2019-12-16T21:21:03Z

@frangio updated!

frangio

LGTM! 🚀

spalladino added 8 commits December 13, 2019 12:37

Move getPlaceholder to utils

cc4c392

Add tuple to getPlaceholder

52c2376

Parse tuples in interactive commands

8b77707

Add test case in ImplV1 contract

96371aa

Show tuple types in interactive prompt

d431c7f

Fix function name parsing when it has a tuple argument

cf380a6

Use parens instead of brackets when parsing tuples

6d2924a

Lint

aa53001

spalladino requested a review from frangio December 13, 2019 20:28

spalladino added 2 commits December 13, 2019 17:28

Remove duplicated call

ab91d3d

Fix tests in prompt

216bfd2

frangio reviewed Dec 16, 2019

View reviewed changes

Implement suggestions by @frangio

d179790

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>

frangio reviewed Dec 16, 2019

View reviewed changes

spalladino added 2 commits December 16, 2019 18:07

Fix partial renaming

70afef8

Fix partial renaming V2

b0eeba0

Merge branch 'master' into feature/support-tuples-input-#1330

20f5c19

frangio approved these changes Dec 17, 2019

View reviewed changes

frangio added the status:ready-to-merge Order mergify to merge label Dec 17, 2019

Lint

c4e3105

mergify bot merged commit e3d1960 into master Dec 17, 2019

mergify bot deleted the feature/support-tuples-input-#1330 branch December 17, 2019 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tuple support in interactive prompts #1333

Add tuple support in interactive prompts #1333

spalladino commented Dec 13, 2019

frangio left a comment

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

frangio Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

spalladino commented Dec 16, 2019

frangio Dec 16, 2019

spalladino Dec 16, 2019

spalladino commented Dec 16, 2019

frangio left a comment

	export function getABIType(arg: any): string {
	export function getABIType(arg: InputInfo): string {

	export function getPlaceholder(arg: Pick<MethodArg, 'type' \| 'components'>): string {
	export function getPlaceholder(arg: Pick<MethodArg, 'type' \| 'components'>): string \| null {

Add tuple support in interactive prompts #1333

Add tuple support in interactive prompts #1333

Conversation

spalladino commented Dec 13, 2019

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Dec 16, 2019

frangio left a comment

Choose a reason for hiding this comment